-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(forge): format new
expressions
#6408
Conversation
crates/fmt/src/formatter.rs
Outdated
Expression::New(_, expr) => { | ||
write!(self.buf(), "new ")?; | ||
self.visit_expr(expr.loc(), expr)?; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nice!
af15350
to
bb0999b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ty!
lgtm
crates/fmt/src/formatter.rs
Outdated
Expression::New(_, expr) => { | ||
write!(self.buf(), "new ")?; | ||
self.visit_expr(expr.loc(), expr)?; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has a smol regression, see failing tests:
ConstructorVictim victim =new ConstructorVictim(
There's a whitespace missing that I think we can check for
I believe before, new expressions where formatted as is, but I like visiting the expression better
The failing workflow indicates that there is potentially still a problem with the implementation. But the last two reported mismatches (in |
agree because previously it formatted using the src directly |
@mattsse I had a deeper look and the problem only happens when the right side of the assignment is a |
I might have, I'll have a look. could be that there's some trim applied |
Ok got it! I was using |
amazing, ty! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gg
3018443
to
d022c3e
Compare
Regarding formatting, do you know why my nightly rustfmt added semicolons for the return statements? I used the nightly from 2023-11-22 |
Ah I figured it out. Actually I formatted wrongly with rustfmt stable at first, which added semicolons, and then switched back to nightly and it kept the semicolons. But if they are not there to begin with it doesn't add them. Maybe there is a way to enforce one or the other in the config file. EDIT: https://rust-lang.github.io/rustfmt/?version=master&search=#trailing_semicolon to enforce them, but there is no way to remove them I think. |
the recent nightly fmt appears to be broken to some extent at least on macos I'm currently using I noticed this 2 weeks ago or so, let's hope gets fixed soon |
The formatter currently doesn't handle
new
expressions, which leads to #4619.This PR fixes that by adding the relevant case in the formatter. The tests have been adjusted to reflect this. They do not pass without the fix, but pass with the fix added.
Please note that the existing test cases had
new uint[](length)
in several places, where the type is not normalized touint256
. In my opinion, the behavior after the fix (convert those touint256
) is correct, and the test files were wrong.Thus, I adjusted all occurences of
new uint[]
in thefmt.sol
files.A new test case for
new Contract()
expressions has been added to theFunctionCall
test, since those expression can be seen as a function call to the constructor.NOTE: cargo fmt changed some formatting in
formatter.rs
, no sure if that's ok.Motivation
The formatter is broken for expression which start with
new
.Solution
Handle the case where an expression is
Expression::New
.Closes #4619